feat: add ComposerService for handling composer.json data #10
Conversation
WalkthroughSwaps PHP-CS-Fixer preset to konradmichalik/php-cs-fixer-preset, adds composer-driven DocBlock header generation, introduces ComposerService, extends DocBlockHeader with fromComposer and __toArray, normalizes token constants in the fixer, updates composer.json/README, and expands tests and headers/licenses across files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CS as php-cs-fixer
participant Preset as Preset (Config/Set)
participant Fixer as DocBlockHeaderFixer
participant Gen as DocBlockHeader (fromComposer)
participant Svc as ComposerService
participant FS as Filesystem
Dev->>CS: run php-cs-fixer
CS->>Preset: load preset (Header::fromComposer, Set::fromArray(...))
CS->>Fixer: apply custom fixer
Fixer->>Gen: request header config (__toArray)
Gen->>Svc: fromComposer(composer.json)
Svc->>FS: read composer.json
FS-->>Svc: JSON contents
Svc-->>Gen: authors/license data
Gen-->>Fixer: annotations array
Fixer->>CS: insert/merge DocBlocks
CS-->>Dev: files fixed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/src/Rules/DocBlockHeaderFixerTest.php (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
tests/src/Enum/SeparateTest.php (1)
24-29: Consider consolidating repetitive enum assertions with data providersOptional: reduce duplication via PHPUnit attributes with a data provider for from/tryFrom/value assertions.
Based on learnings
Example:
use PHPUnit\Framework\Attributes\DataProvider; #[DataProvider('casesProvider')] public function testFromStringProvider(string $in, Separate $expected): void { self::assertSame($expected, Separate::from($in)); } public static function casesProvider(): array { return [ ['top', Separate::Top], ['bottom', Separate::Bottom], ['both', Separate::Both], ['none', Separate::None], ]; }src/Generators/Generator.php (1)
16-21: Minor naming nit: avoid “__” prefix for non-magic methodsOptional: consider renaming __toArray() to toArray() to prevent confusion with PHP magic methods.
For clarity, the interface could be:
interface Generator { /** @return array<string, array<string, mixed>> */ public function toArray(): array; }README.md (1)
113-127: Clarify defaults and failure modes for fromComposer()
- fromComposer() will throw if composer.json is missing/invalid. Consider a short note or path override hint.
- Also, generator default add_structure_name is true, while the fixer option’s documented default is false below. Consider clarifying that the generator sets it explicitly (so users aren’t surprised).
src/Rules/DocBlockHeaderFixer.php (1)
339-346: Support hyphenated tags when parsing existing annotationsparseExistingAnnotations ignores tags like property-read and used-by. Widen the pattern.
Apply this diff:
- if (preg_match('/^@(\w+)(?:\s+(.*))?$/', $line, $matches)) { + if (preg_match('/^@([A-Za-z][A-Za-z0-9_-]*)(?:\s+(.*))?$/', $line, $matches)) {src/Service/ComposerService.php (1)
49-56: Use JSON_THROW_ON_ERROR for clearer failuresImproves error messages and avoids silent nulls from json_decode.
Apply this diff:
- $data = json_decode($contents, true); - - if (!is_array($data)) { - throw new RuntimeException(sprintf('Unable to decode JSON from file "%s".', $composerJsonPath)); - } + try { + $data = json_decode($contents, true, 512, \JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + throw new RuntimeException(sprintf('Unable to decode JSON from file "%s": %s', $composerJsonPath, $e->getMessage()), 0, $e); + } + if (!is_array($data)) { + throw new RuntimeException(sprintf('Unexpected JSON root type in "%s": %s', $composerJsonPath, gettype($data))); + }tests/src/Service/ComposerServiceTest.php (3)
81-83: Nit: correct comment (warning, not notice).- // Suppress the expected PHP notice when reading a directory + // Suppress the expected PHP warning when reading a directory
20-29: Merge duplicated docblocks; keep a single class docblock after the attribute.The standalone “@internal” docblock is detached and not applied to the class. Merge it into the main docblock.
-/** - * @internal - */ -#[\PHPUnit\Framework\Attributes\CoversClass(ComposerService::class)] -/** - * ComposerServiceTest. - * - * @author Konrad Michalik <hej@konradmichalik.dev> - * @license GPL-3.0-or-later - */ +#[\PHPUnit\Framework\Attributes\CoversClass(ComposerService::class)] +/** + * ComposerServiceTest. + * + * @internal + * @author Konrad Michalik <hej@konradmichalik.dev> + * @license GPL-3.0-or-later + */
36-37: Prefer tempnam() for safer temp filenames.- $this->testComposerJsonPath = sys_get_temp_dir() . '/test-composer-' . uniqid() . '.json'; + $tmp = tempnam(sys_get_temp_dir(), 'composer-'); + self::assertNotFalse($tmp); + $this->testComposerJsonPath = $tmp;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.php-cs-fixer.php(1 hunks)README.md(1 hunks)composer.json(2 hunks)rector.php(1 hunks)src/Enum/Separate.php(1 hunks)src/Generators/DocBlockHeader.php(2 hunks)src/Generators/Generator.php(1 hunks)src/Rules/DocBlockHeaderFixer.php(12 hunks)src/Service/ComposerService.php(1 hunks)tests/src/Enum/SeparateTest.php(2 hunks)tests/src/Generators/DocBlockHeaderTest.php(4 hunks)tests/src/Rules/DocBlockHeaderFixerTest.php(10 hunks)tests/src/Service/ComposerServiceTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Generators/DocBlockHeader.php (2)
src/Service/ComposerService.php (4)
ComposerService(30-107)readComposerJson(37-56)extractAuthors(77-94)extractLicense(61-70)src/Generators/Generator.php (1)
__toArray(27-27)
tests/src/Generators/DocBlockHeaderTest.php (1)
src/Generators/DocBlockHeader.php (2)
DocBlockHeader(32-150)fromComposer(74-115)
tests/src/Rules/DocBlockHeaderFixerTest.php (1)
src/Rules/DocBlockHeaderFixer.php (2)
getPriority(55-60)configure(96-99)
tests/src/Service/ComposerServiceTest.php (1)
src/Service/ComposerService.php (5)
ComposerService(30-107)readComposerJson(37-56)extractLicense(61-70)extractAuthors(77-94)getPrimaryAuthor(101-106)
.php-cs-fixer.php (3)
src/Generators/DocBlockHeader.php (4)
DocBlockHeader(32-150)create(60-69)fromComposer(74-115)__toArray(45-55)src/Rules/DocBlockHeaderFixer.php (1)
DocBlockHeaderFixer(35-420)src/Generators/Generator.php (1)
__toArray(27-27)
🪛 GitHub Actions: Tests
tests/src/Rules/DocBlockHeaderFixerTest.php
[error] 958-958: ParseError: syntax error, unexpected token "readonly" during test execution in anonymous class. Occurred while running 'composer test' (PHPUnit).
[error] 980-980: ParseError: syntax error, unexpected token "readonly" during test execution in anonymous class. Occurred while running 'composer test' (PHPUnit).
🪛 GitHub Check: cgl / cgl
tests/src/Service/ComposerServiceTest.php
[warning] 1-1:
Found violation(s) of type: concat_space
🔇 Additional comments (13)
tests/src/Enum/SeparateTest.php (1)
6-11: Header/license standardization LGTMConsistent with the repo-wide header refactor.
src/Enum/Separate.php (1)
6-11: Docs/header updates LGTMEnum doc and header align with new policy; no behavior change.
Also applies to: 16-21
src/Generators/Generator.php (1)
6-11: Header/license standardization LGTMMatches project-wide convention.
rector.php (1)
6-11: Header/license standardization LGTMNo functional changes; config remains intact.
composer.json (2)
43-47: PSR-4 autoload-dev mapping LGTMNamespace matches tests/src; remember to run composer dump-autoload.
32-32: Run Composer compatibility checks
Ensurekonradmichalik/php-cs-fixer-preset:^0.1.0andfriendsofphp/php-cs-fixer:^3.14don’t conflict with your PHP matrix. Locally execute:composer why-not konradmichalik/php-cs-fixer-preset ^0.1.0 composer why-not friendsofphp/php-cs-fixer ^3.14 composer prohibits konradmichalik/php-cs-fixer-preset:^0.1.0 friendsofphp/php-cs-fixer:^3.14 composer update --dry-run konradmichalik/php-cs-fixer-preset friendsofphp/php-cs-fixertests/src/Generators/DocBlockHeaderTest.php (1)
308-482: Solid coverage for fromComposer scenariosCovers single/multiple/no author, missing license, extra annotations, and custom flags. Looks good.
src/Rules/DocBlockHeaderFixer.php (1)
111-117: Token constant refactor LGTMUsing fully-qualified token constants improves clarity and avoids import ambiguity.
.php-cs-fixer.php (1)
14-34: Verify preset dependency and rule compatibilityEnsure konradmichalik/php-cs-fixer-preset is present and Set::fromArray(...) accepts the generator’s array as intended. Also confirm CI uses PHP ≥ 8.1 (enums, token constants).
tests/src/Rules/DocBlockHeaderFixerTest.php (1)
53-56: Priority assertion matches implementationGood to lock getPriority() to 1.
src/Generators/DocBlockHeader.php (2)
60-69: Default addStructureName=true – confirm intentcreate() now defaults addStructureName to true; tests and README examples align, but the fixer’s option default remains false. Confirm this divergence is intentional to keep OO/generator path verbose by default.
74-115: fromComposer() implementation LGTMCorrectly builds author(s) and license, merges additional annotations post-compute, and delegates to create().
tests/src/Service/ComposerServiceTest.php (1)
1-1: Style: concat_space violation flagged by CGL.Static analysis reported concat_space. Please run your fixer (php-cs-fixer/cgl) and align concatenation spacing with the repo’s rule (one or none).
| if ($token->isGivenKind(\T_DOC_COMMENT)) { | ||
| return $i; | ||
| } | ||
|
|
||
| // If we hit any other meaningful token (except modifiers), stop looking | ||
| if (!$token->isGivenKind([T_FINAL, T_ABSTRACT, T_ATTRIBUTE])) { | ||
| if (!$token->isGivenKind([\T_FINAL, \T_ABSTRACT, \T_ATTRIBUTE])) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Treat T_READONLY as a class modifier in lookback/insert logic
Readonly classes (PHP 8.2+) can have a T_READONLY before T_CLASS. Not handling it may miss existing DocBlocks and insert in the wrong spot.
Apply this diff:
- if (!$token->isGivenKind([\T_FINAL, \T_ABSTRACT, \T_ATTRIBUTE])) {
+ if (!$token->isGivenKind([\T_FINAL, \T_ABSTRACT, \T_ATTRIBUTE, \T_READONLY])) {
break;
}- if ($token->isGivenKind([\T_FINAL, \T_ABSTRACT, \T_ATTRIBUTE])) {
+ if ($token->isGivenKind([\T_FINAL, \T_ABSTRACT, \T_ATTRIBUTE, \T_READONLY])) {
$insertIndex = $i;
continue;
}Also applies to: 318-321
🤖 Prompt for AI Agents
In src/Rules/DocBlockHeaderFixer.php around lines 221-229 (and also update the
similar check at lines 318-321), the lookback/insert logic treats class
modifiers but currently omits T_READONLY; add \T_READONLY to the array of
modifier token kinds so readonly classes are recognized as modifiers and the
DocBlock lookup/insert point is correct. Make the exact change in both locations
by including \T_READONLY in the list passed to isGivenKind.
| public function testIsAnonymousClassWithReadonlyModifier(): void | ||
| { | ||
| $code = '<?php $obj = new readonly class {};'; | ||
| $tokens = Tokens::fromCode($code); | ||
|
|
||
| $method = new ReflectionMethod($this->fixer, 'isAnonymousClass'); | ||
|
|
||
| // Find the class token index | ||
| $classIndex = null; | ||
| for ($i = 0; $i < $tokens->count(); ++$i) { | ||
| if ($tokens[$i]->isGivenKind(\T_CLASS)) { | ||
| $classIndex = $i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| $result = $method->invoke($this->fixer, $tokens, $classIndex); | ||
|
|
||
| // This tests line 156: continue when token is T_READONLY or T_FINAL | ||
| self::assertTrue($result); | ||
| } |
There was a problem hiding this comment.
Fix CI parse errors: gate “readonly class” tests to PHP ≥ 8.2
On PHP < 8.2, tokenizing '<?php $obj = new readonly class {};' fails. Gate these tests to avoid ParseError.
Apply this diff:
public function testIsAnonymousClassWithReadonlyModifier(): void
{
+ if (\PHP_VERSION_ID < 80200) {
+ self::markTestSkipped('Requires PHP 8.2+ (readonly classes).');
+ }
$code = '<?php $obj = new readonly class {};';
$tokens = Tokens::fromCode($code);
$method = new ReflectionMethod($this->fixer, 'isAnonymousClass');
// Find the class token index
$classIndex = null;
for ($i = 0; $i < $tokens->count(); ++$i) {
if ($tokens[$i]->isGivenKind(\T_CLASS)) {
$classIndex = $i;
break;
}
}
$result = $method->invoke($this->fixer, $tokens, $classIndex);
// This tests line 156: continue when token is T_READONLY or T_FINAL
self::assertTrue($result);
}
public function testSkipsAnonymousClassWithReadonlyModifier(): void
{
+ if (\PHP_VERSION_ID < 80200) {
+ self::markTestSkipped('Requires PHP 8.2+ (readonly classes).');
+ }
$code = '<?php $obj = new readonly class {};';
$tokens = Tokens::fromCode($code);
$file = new SplFileInfo(__FILE__);
$method = new ReflectionMethod($this->fixer, 'applyFix');
$this->fixer->configure([
'annotations' => ['author' => 'John Doe'],
'separate' => 'none',
'ensure_spacing' => false,
]);
$method->invoke($this->fixer, $file, $tokens);
// Anonymous class with readonly modifier should NOT have DocBlock added
self::assertSame($code, $tokens->generateCode());
}Alternatively, use #[\PHPUnit\Framework\Attributes\RequiresPhp('>= 8.2')]. Based on learnings
Also applies to: 977-994
🧰 Tools
🪛 GitHub Actions: Tests
[error] 958-958: ParseError: syntax error, unexpected token "readonly" during test execution in anonymous class. Occurred while running 'composer test' (PHPUnit).
🤖 Prompt for AI Agents
In tests/src/Rules/DocBlockHeaderFixerTest.php around lines 955-975 (and
similarly 977-994), the test that tokenizes '<?php $obj = new readonly class
{};' fails on PHP < 8.2; update the test to gate it to PHP >= 8.2 by adding a
PHPUnit requirement or runtime skip: either annotate the test with
#[\PHPUnit\Framework\Attributes\RequiresPhp('>= 8.2')] or add an early runtime
check that calls $this->markTestSkipped('requires PHP >= 8.2') when
PHP_VERSION_ID < 80200, so the test only runs on PHP 8.2+ and avoids ParseError
on older interpreters.
| $this->expectException(RuntimeException::class); | ||
| $this->expectExceptionMessage('Unable to read file'); | ||
| // Suppress the expected PHP warning when reading an unreadable file | ||
| @ComposerService::readComposerJson($invalidPath); |
There was a problem hiding this comment.
Fix fragile exception assertion (exact message mismatch).
Exception message includes the file path; use a regex match.
- $this->expectException(RuntimeException::class);
- $this->expectExceptionMessage('Unable to read file');
+ $this->expectException(RuntimeException::class);
+ $this->expectExceptionMessageMatches('/Unable to read file/');🤖 Prompt for AI Agents
In tests/src/Service/ComposerServiceTest.php around lines 100 to 103, the test
currently asserts an exact exception message but the actual message includes the
file path so the assert is brittle; replace the exact-message assertion with a
regex-based assertion (use PHPUnit's expectExceptionMessageMatches and construct
a pattern that matches "Unable to read file" and the optional path — build the
pattern using preg_quote($invalidPath, '/') so the path is escaped) and keep the
rest of the test (including the warning suppression) unchanged.
| $this->expectException(RuntimeException::class); | ||
| $this->expectExceptionMessage('Unable to decode JSON'); | ||
|
|
||
| ComposerService::readComposerJson($this->testComposerJsonPath); | ||
| } |
There was a problem hiding this comment.
Fix invalid JSON assertion to allow dynamic file path.
Match the invariant part of the message.
- $this->expectException(RuntimeException::class);
- $this->expectExceptionMessage('Unable to decode JSON');
+ $this->expectException(RuntimeException::class);
+ $this->expectExceptionMessageMatches('/Unable to decode JSON/');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->expectException(RuntimeException::class); | |
| $this->expectExceptionMessage('Unable to decode JSON'); | |
| ComposerService::readComposerJson($this->testComposerJsonPath); | |
| } | |
| $this->expectException(RuntimeException::class); | |
| $this->expectExceptionMessageMatches('/Unable to decode JSON/'); | |
| ComposerService::readComposerJson($this->testComposerJsonPath); | |
| } |
🤖 Prompt for AI Agents
In tests/src/Service/ComposerServiceTest.php around lines 119-123, the test
currently asserts the full exception message which includes a dynamic file path;
replace the strict equality assertion with a pattern match so only the invariant
part is checked. Specifically, change the expectExceptionMessage(...) call to
use expectExceptionMessageMatches('/Unable to decode JSON/') (or an equivalent
regex-based expectation) so the test accepts messages containing that phrase
plus any dynamic path.
Pull Request Test Coverage Report for Build 18461941382Details
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/Rules/DocBlockHeaderFixer.php (2)
24-25: Consider removing redundant function imports.Importing
in_arrayandis_arrayis valid but unnecessary, as these functions are already available globally in PHP. This adds no functional benefit and is uncommon in PHP codebases.If you prefer this style for consistency or IDE support, it's acceptable to keep. Otherwise, consider removing:
-use function in_array; -use function is_array; -
64-67: Consider reverting to standard token constant syntax.Throughout this file, token constants have been changed from
T_CLASSto\T_CLASS(and similarly for other constants). While technically correct, this is inconsistent with standard PHP practices. Token constants are already in the global namespace and the backslash prefix is redundant.Unless there's a specific reason for this change (e.g., compatibility with a static analysis tool or coding standard), consider reverting to the standard syntax:
-if ($token->isGivenKind(\T_CLASS)) { +if ($token->isGivenKind(T_CLASS)) {Apply this pattern throughout the file for all token constants.
Also applies to: 111-111, 116-116, 144-144, 155-155, 160-160, 201-201, 221-221, 226-226, 244-244, 259-259, 280-280, 285-285, 291-291, 299-299, 318-318, 416-416
tests/src/Service/ComposerServiceTest.php (1)
71-86: Consider the test behavior for reading a directory.The test uses a directory to simulate an unreadable file, but the behavior may vary by platform:
- On some systems,
file_get_contents()on a directory might return a warning and false- On others, it might succeed with empty or garbage content that fails JSON decode
The regex pattern accounts for this variation, but the test name suggests it's testing "CannotBeRead" when it might actually be testing decode failure.
Consider either:
- Renaming to better reflect the actual test scenario (e.g.,
testReadComposerJsonThrowsExceptionOnInvalidInput)- Or documenting the platform-specific behavior in a comment
src/Generators/DocBlockHeader.php (1)
74-115: Clarify annotation override behavior.The implementation correctly extracts composer.json metadata and formats it into annotations. However, line 112's merge strategy means
$additionalAnnotationscan override extracted author/license values.Consider documenting this behavior in the method's PHPDoc:
/** * @param array<string, string|array<string>> $additionalAnnotations + * + * Note: $additionalAnnotations will override any annotations extracted from composer.json + * if they have the same key (e.g., 'author', 'license'). */ public static function fromComposer(This makes the override behavior explicit for users who might want to supplement rather than replace certain annotations.
src/Service/ComposerService.php (1)
37-56: Consider more specific JSON error handling.The implementation correctly validates file existence, read success, and JSON decoding. However, you could provide more specific error messages by checking
json_last_error().$data = json_decode($contents, true); -if (!is_array($data)) { - throw new RuntimeException(sprintf('Unable to decode JSON from file "%s".', $composerJsonPath)); +if (JSON_ERROR_NONE !== json_last_error()) { + throw new RuntimeException(sprintf( + 'Unable to decode JSON from file "%s": %s', + $composerJsonPath, + json_last_error_msg() + )); +} + +if (!is_array($data)) { + throw new RuntimeException(sprintf('Invalid JSON structure in file "%s": expected object, got %s.', $composerJsonPath, gettype($data))); }This provides more actionable error messages for debugging, though the current implementation is acceptable for the use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.php-cs-fixer.php(1 hunks)README.md(1 hunks)composer.json(2 hunks)rector.php(1 hunks)src/Enum/Separate.php(1 hunks)src/Generators/DocBlockHeader.php(2 hunks)src/Generators/Generator.php(1 hunks)src/Rules/DocBlockHeaderFixer.php(12 hunks)src/Service/ComposerService.php(1 hunks)tests/src/Enum/SeparateTest.php(2 hunks)tests/src/Generators/DocBlockHeaderTest.php(4 hunks)tests/src/Rules/DocBlockHeaderFixerTest.php(10 hunks)tests/src/Service/ComposerServiceTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/src/Generators/DocBlockHeaderTest.php (2)
src/Generators/DocBlockHeader.php (2)
DocBlockHeader(32-150)fromComposer(74-115)tests/src/Service/ComposerServiceTest.php (1)
PHPUnit(23-259)
src/Generators/DocBlockHeader.php (2)
src/Service/ComposerService.php (4)
ComposerService(30-107)readComposerJson(37-56)extractAuthors(77-94)extractLicense(61-70)src/Generators/Generator.php (1)
__toArray(27-27)
tests/src/Service/ComposerServiceTest.php (1)
src/Service/ComposerService.php (5)
ComposerService(30-107)readComposerJson(37-56)extractLicense(61-70)extractAuthors(77-94)getPrimaryAuthor(101-106)
tests/src/Rules/DocBlockHeaderFixerTest.php (1)
src/Rules/DocBlockHeaderFixer.php (2)
getPriority(55-60)configure(96-99)
.php-cs-fixer.php (3)
src/Generators/DocBlockHeader.php (4)
DocBlockHeader(32-150)create(60-69)fromComposer(74-115)__toArray(45-55)src/Rules/DocBlockHeaderFixer.php (1)
DocBlockHeaderFixer(35-420)src/Generators/Generator.php (1)
__toArray(27-27)
🪛 GitHub Actions: Tests
tests/src/Rules/DocBlockHeaderFixerTest.php
[error] 959-959: ParseError: syntax error, unexpected token "readonly"
[error] 982-982: ParseError: syntax error, unexpected token "readonly"
🔇 Additional comments (24)
src/Enum/Separate.php (1)
5-21: LGTM! Header standardization applied correctly.The updated file header and class-level docblock align with the new standardized format being applied across the codebase. No functional changes to the enum itself.
tests/src/Enum/SeparateTest.php (1)
5-29: LGTM! Test file header standardization applied correctly.The header and class-level docblock updates are consistent with the standardization being applied across the codebase.
README.md (1)
113-127: LGTM! Clear documentation of the new fromComposer feature.The usage example effectively demonstrates how to automatically extract author and license information from composer.json. This is a valuable addition that shows the simplest configuration pattern.
src/Generators/Generator.php (1)
5-21: LGTM! Interface documentation standardized.The header and interface-level docblock updates are consistent with the standardization being applied across the codebase. No changes to the public interface signature.
rector.php (1)
5-12: LGTM! Configuration file header standardized.The header update is consistent with the standardization being applied across the codebase.
composer.json (2)
43-47: LGTM! Autoload-dev mapping added correctly.The PSR-4 autoload-dev mapping for the test namespace is correctly configured and enables proper test discovery for the new test files.
32-32: composer.json:konradmichalik/php-cs-fixer-presetis available on Packagist (v0.1.0) and the autoload-dev mapping is correct.tests/src/Generators/DocBlockHeaderTest.php (4)
18-18: LGTM! Import statement modernized with group syntax.The grouped import for DocBlockHeader and Generator is a cleaner approach and aligns with modern PHP style.
26-31: LGTM! Test class documentation added.The class-level docblock is consistent with the standardization being applied across the codebase.
307-483: LGTM! Comprehensive test coverage for fromComposer functionality.The new test methods provide excellent coverage of the fromComposer feature:
- Single and multiple authors with/without emails
- Missing authors or license handling
- Additional annotations merging
- Custom parameter configurations
All tests properly create temporary files and clean them up in finally blocks, following best practices.
89-89: Tests explicitly configureadd_structure_name; no default changed. The generator’screate()andfromComposer()methods have always defaulted totrue, and the fixer option default remainsfalseas documented. The tests aren’t relying on a new default but pass the flag explicitly, so there is no breaking change.Likely an incorrect or invalid review comment.
src/Rules/DocBlockHeaderFixer.php (3)
5-12: LGTM! Header standardized.The header update is consistent with the standardization being applied across the codebase.
19-21: LGTM! Grouped imports improve readability.Consolidating the PhpCsFixer imports using grouped syntax makes the import section more compact and organized.
28-31: LGTM! Class-level documentation added.The docblock is consistent with the standardization being applied across the codebase.
.php-cs-fixer.php (2)
21-34: LGTM!The configuration structure is well-organized:
- Custom fixer registration is properly implemented
- The use of
fromComposer()aligns with the PR's objective to enable composer.json-driven header generation- The
__toArray()method correctly serializes the configuration for the preset system- Finder setup is appropriate
14-19: Dependency availability confirmedConfirmed
konradmichalik/php-cs-fixer-presetexists on Packagist at version ^0.1.0 (published 2025-10-08), matching composer.json.tests/src/Service/ComposerServiceTest.php (3)
34-44: LGTM!The setup and teardown methods properly ensure test isolation by creating unique temporary files and cleaning up after each test.
125-232: LGTM!Comprehensive test coverage for license and author extraction:
- Handles all expected data shapes (string, array, missing)
- Properly filters invalid entries
- Uses appropriate PHPStan suppressions for optional fields in structured arrays
234-258: LGTM!The tests properly verify that
getPrimaryAuthorreturns the first author when multiple exist and null when none are present.tests/src/Rules/DocBlockHeaderFixerTest.php (1)
53-56: LGTM!The test properly verifies that
getPriority()returns the expected value of 1, ensuring the fixer runs before other relevant fixers.src/Generators/DocBlockHeader.php (1)
42-55: LGTM!The
__toArray()method correctly serializes the DocBlockHeader configuration in a format compatible with the PHP-CS-Fixer preset system, matching the fixer name used inDocBlockHeaderFixer.src/Service/ComposerService.php (3)
61-70: LGTM!The method correctly handles the Composer schema for licenses:
- Returns null when license is not specified
- Handles single license string
- Returns the first license from an array
- Gracefully handles empty arrays
77-94: LGTM!The method properly extracts and validates authors according to the Composer schema:
- Returns empty array for missing or invalid authors field
- Filters out invalid entries (non-arrays or missing name)
- Preserves optional fields (email, role)
- Returns a properly typed list
101-106: LGTM!The method correctly delegates to
extractAuthors()and returns the first author or null. The implementation is clean and reuses existing validation logic.
| public static function create( | ||
| array $annotations, | ||
| bool $preserveExisting = true, | ||
| Separate $separate = Separate::Both, | ||
| bool $addStructureName = false, | ||
| bool $addStructureName = true, | ||
| ): self { | ||
| self::validateAnnotations($annotations); | ||
|
|
||
| return new self($annotations, $preserveExisting, $separate, $addStructureName); | ||
| } |
There was a problem hiding this comment.
Note: Breaking change in default behavior.
The default value of $addStructureName has changed from false to true. This aligns the create() method with fromComposer() for consistency, but it changes the default behavior for existing code that relies on the previous default.
Consider:
- Documenting this breaking change in the release notes or changelog
- Evaluating whether this should be a major version bump according to semantic versioning
The change improves API consistency, but users who called create() without explicitly setting addStructureName will now get different output.
🤖 Prompt for AI Agents
In src/Generators/DocBlockHeader.php around lines 60 to 69 the default for
$addStructureName was changed to true which is a breaking behavioral change;
either revert the default back to false to preserve existing behavior or, if you
intentionally want the new default, document the change in the release
notes/changelog and increment the major version per semver; update unit tests
and any examples to reflect the chosen default and add a short changelog entry
noting the change in create() default for $addStructureName.
| #[\PHPUnit\Framework\Attributes\RequiresPhp('>= 8.2')] | ||
| public function testIsAnonymousClassWithReadonlyModifier(): void | ||
| { | ||
| $code = '<?php $obj = new readonly class {};'; | ||
| $tokens = Tokens::fromCode($code); | ||
|
|
||
| $method = new ReflectionMethod($this->fixer, 'isAnonymousClass'); | ||
|
|
||
| // Find the class token index | ||
| $classIndex = null; | ||
| for ($i = 0; $i < $tokens->count(); ++$i) { | ||
| if ($tokens[$i]->isGivenKind(\T_CLASS)) { | ||
| $classIndex = $i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| $result = $method->invoke($this->fixer, $tokens, $classIndex); | ||
|
|
||
| // This tests line 156: continue when token is T_READONLY or T_FINAL | ||
| self::assertTrue($result); | ||
| } | ||
|
|
||
| #[\PHPUnit\Framework\Attributes\RequiresPhp('>= 8.2')] | ||
| public function testSkipsAnonymousClassWithReadonlyModifier(): void | ||
| { | ||
| $code = '<?php $obj = new readonly class {};'; | ||
| $tokens = Tokens::fromCode($code); | ||
| $file = new SplFileInfo(__FILE__); | ||
|
|
||
| $method = new ReflectionMethod($this->fixer, 'applyFix'); | ||
|
|
||
| $this->fixer->configure([ | ||
| 'annotations' => ['author' => 'John Doe'], | ||
| 'separate' => 'none', | ||
| 'ensure_spacing' => false, | ||
| ]); | ||
| $method->invoke($this->fixer, $file, $tokens); | ||
|
|
||
| // Anonymous class with readonly modifier should NOT have DocBlock added | ||
| self::assertSame($code, $tokens->generateCode()); | ||
| } |
There was a problem hiding this comment.
Fix parse errors on PHP < 8.2.
The pipeline failures indicate that the readonly keyword in the test code causes parse errors on PHP versions below 8.2, even though the tests have RequiresPhp('>= 8.2') attributes. PHP parses the entire file before checking attributes, so syntax errors occur during parsing.
To fix this, use one of these approaches:
Solution 1: Use heredoc syntax (preferred)
- $code = '<?php $obj = new readonly class {};';
+ $code = <<<'PHP'
+<?php $obj = new readonly class {};
+PHP;Solution 2: Dynamic string construction
- $code = '<?php $obj = new readonly class {};';
+ $code = '<?php $obj = new ' . 'readonly' . ' class {};';Solution 3: Conditional test definition
if (PHP_VERSION_ID >= 80200) {
// Define the test method here
}The heredoc approach is cleanest and maintains readability while avoiding parse errors.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 959-959: ParseError: syntax error, unexpected token "readonly"
[error] 982-982: ParseError: syntax error, unexpected token "readonly"
Summary by CodeRabbit
New Features
Documentation
Tests
Style
Chores